Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Missing embedded ansible persisters dependencies #17574

Merged

Conversation

slemrmartin
Copy link
Contributor

@slemrmartin slemrmartin commented Jun 13, 2018

From old to new interface

Fixes problem https://travis-ci.org/ManageIQ/manageiq/jobs/391452668

ManageIQ::Providers::EmbeddedAnsible::Inventory::Persister::AutomationManager / ConfigurationScriptSource includes removed mixins (ManageIQ/manageiq-providers-ansible_tower#81)

Reverting fix: ManageIQ/manageiq-providers-ansible_tower#97 (this is intended to replace them)

@slemrmartin
Copy link
Contributor Author

@miq-bot add_label bug
Cc @Ladas @jameswnl @agrare

@miq-bot miq-bot added the bug label Jun 13, 2018
@slemrmartin slemrmartin mentioned this pull request Jun 13, 2018
33 tasks
@slemrmartin slemrmartin changed the title Fixes embedded ansible persisters dependencies Missing embedded ansible persisters dependencies Jun 13, 2018
@slemrmartin
Copy link
Contributor Author

@jameswnl it seems it is failing in one test: https://travis-ci.org/ManageIQ/manageiq/jobs/391635305

I'm not sure how this error happens, local test is ok.
Is this check necessary? That class exists and is identical

@Ladas
Copy link
Contributor

Ladas commented Jun 13, 2018

@jameswnl @slemrmartin looks to me that the correct STI class should be ManageIQ::Providers::EmbeddedAutomationManager::InventoryRootGroup

@Ladas
Copy link
Contributor

Ladas commented Jun 13, 2018

@jameswnl @slemrmartin actually, to be consistent with other providers we should be using ManageIQ::Providers::EmbeddedAnsible::AutomationManager::InventoryRootGroup

Also its seems like we do have a lot of empty abstract classes there, so the ansible codebase would deserve a cleanup. :-)

@agrare agrare self-assigned this Jun 13, 2018
@jameswnl
Copy link
Contributor

@Ladas I can't figure out why this line would resolve to ManageIQ::Providers::AutomationManager::InventoryRootGroup

The others in the same file are properly resolving to ManageIQ::Providers::EmbeddedAnsible::AutomationManager::*

@slemrmartin
Copy link
Contributor Author

@jameswnl it seems like problem is in spec, not in my code, right?
So I would like to change spec to :type => manager_class::InventoryRootGroup.name like all other specs.

But I don't know how to simulate travis error, do you know?

@agrare
Copy link
Member

agrare commented Jun 25, 2018

@jameswnl @Ladas what class do we expect these to be as a result of a refresh today?

@jameswnl
Copy link
Contributor

@slemrmartin yeah, I think so. But the problem I have is same as you - don't know how to reproduce travis error locally.

@slemrmartin
Copy link
Contributor Author

travis fixed in ManageIQ/manageiq-providers-ansible_tower#104
but it depends on this PR.
@agrare what next?

Copy link
Contributor

@Ladas Ladas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@agrare in other providers we always have STI classes aligned with the manager class (except for few storage providers). But here, we don't have those classes defined for some reason.

@slemrmartin if it's cyclical @agrare will have to merge them at the same time

@agrare
Copy link
Member

agrare commented Jul 9, 2018

@slemrmartin @Ladas @jameswnl I think the problem is that the old interface was setting the STI class incorrectly. Even though it is "wrong" without a data migration to correct it the new interface should be compatible with the old interface.

This new interface should set the same STI classes as the old one even if the old one wasn't correct. If you put a PR in to fix existing data then we can fix it here.

@slemrmartin slemrmartin force-pushed the embedded-ansible-persisters-dependency branch from ce7274b to 7358e42 Compare July 18, 2018 10:21
manager module forced to AutomationManager
@slemrmartin slemrmartin force-pushed the embedded-ansible-persisters-dependency branch from 7358e42 to 84de5d2 Compare July 18, 2018 11:48
@slemrmartin
Copy link
Contributor Author

@agrare do you want to see Rails in action? :)
"ManageIQ::Providers::EmbeddedAnsible::AutomationManager::InventoryRootGroup".safe_constantize ==

  • ManageIQ::Providers::AutomationManager::InventoryRootGroup on dev
  • ManageIQ::Providers::EmbeddedAutomationManager::InventoryRootGroup in travis

@slemrmartin slemrmartin force-pushed the embedded-ansible-persisters-dependency branch from 9967656 to 71875db Compare July 18, 2018 14:08
@slemrmartin
Copy link
Contributor Author

@jameswnl because InventoryRootGroup is the only inventory collection which doesn't have class under all namespaces, it's the only IC where ManageIQ::Providers::AutomationManager::InventoryRootGroup have to be specified explicitely.

A weird behaviour I wrote above is performed also for new method. For EmbeddedAnsible it was unpredictable which class is chosen by autoloader.

So I think these specs worked only with some luck before.

Safe_constantize can autoload different classes than expected.
@slemrmartin slemrmartin force-pushed the embedded-ansible-persisters-dependency branch from 71875db to e67709b Compare July 18, 2018 15:01
@miq-bot
Copy link
Member

miq-bot commented Jul 18, 2018

Checked commits slemrmartin/manageiq@617b637~...e67709b with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 👍

@agrare agrare reopened this Jul 20, 2018
@agrare agrare merged commit 3e58954 into ManageIQ:master Jul 20, 2018
@agrare agrare added this to the Sprint 91 Ending Jul 30, 2018 milestone Jul 20, 2018
@slemrmartin slemrmartin deleted the embedded-ansible-persisters-dependency branch July 23, 2018 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants